Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f12577e085
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
packages/super-editor/src/extensions/custom-selection/custom-selection.js
Outdated
Show resolved
Hide resolved
caio-pizzol
left a comment
caio-pizzol
left a comment
There was a problem hiding this comment.
@VladaHarbour good approach — wrapping undo/redo to clear the ghost highlight makes sense.
main question: the fix also collapses the cursor after every undo/redo, not just the ghost highlight case. could it work without that part? also, the collab undo path skips the fix entirely. left inline comments with details.
on DX: one object literal duplicates DEFAULT_SELECTION_STATE (drift risk if the shape changes), and a dead typeof guard can be simplified — both minor.
test coverage: redo path uses the same wrapper but has no test, and the new setOptions call in the non-toolbar blur branch isn't covered.
| @@ -55,7 +88,8 @@ export const History = Extension.create({ | |||
| return yUndo(state); | |||
There was a problem hiding this comment.
collab undo (yUndo) returns before this cleanup runs, so the ghost highlight bug would still happen in collab sessions. does it need to be handled there too?
| }); | ||
|
|
||
| const sel = cleared.selection; | ||
| if (sel && sel instanceof TextSelection && !sel.empty) { |
There was a problem hiding this comment.
this removes the text selection after every undo/redo. most editors keep the restored text selected so users see what changed. is that ok here, or could we skip this part and just clear the highlight state (steps 1 and 3)?
| if (!dispatch) return dispatch; | ||
|
|
||
| return (historyTr) => { | ||
| let cleared = historyTr.setMeta(CustomSelectionPluginKey, { |
There was a problem hiding this comment.
this object is the same as DEFAULT_SELECTION_STATE in custom-selection.js. if one changes and the other doesn't, they'll get out of sync. worth importing it instead?
|
|
||
| const sel = cleared.selection; | ||
| if (sel && sel instanceof TextSelection && !sel.empty) { | ||
| const headPos = typeof sel.head === 'number' ? sel.head : sel.to; |
There was a problem hiding this comment.
after the instanceof TextSelection check above, sel.head is always a number, so the typeof check and sel.to fallback never run. can simplify to just TextSelection.create(cleared.doc, sel.head).
| // Also clear editor-level preserved selection snapshots so that | ||
| // subsequent commands (linked styles, mark commands, etc.) don't | ||
| // resurrect an old selection after history undo/redo. | ||
| this.editor.setOptions({ |
There was a problem hiding this comment.
this new clearing code doesn't have a test. the existing blur tests don't hit this branch.
3 participants
No description provided.